-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🐛 Controller: Wait for all reconciliations before shutting down #1427
Conversation
Currently, the controller will instantly shutdown and return when its context gets cancelled, leaving active reconciliations be. This change makes it wait for those before finishing shutdown.
for i := 0; i < c.MaxConcurrentReconciles; i++ { | ||
go wait.UntilWithContext(ctx, func(ctx context.Context) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I can see, the wait.UntilWithContext
was harmless, but not useful because it would start the closure every c.JitterPeriod
for as long as the ctx
is not cancelled. The closure will however stay in the for c.processNextWorkItem(ctx)
loop until the workqueue
is shutdown and the workqueue
is only shutdown when the ctx
is cancelled.
I think we still have this because before b4a0212 it was possible for the closure to end.
With the changes in this PR, using wait.UntilWithContext
may lead to a deadlock:
- We add the workers to the
WaitGroup
- Context gets cancelled before the closure is started
wait.UntilWithContext
will not start the closure so nothing will ever callwg.Done()
- The
wg.Wait
is deadlocked
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes lots of sense, and it'd actually explain why I was still noticing some errors during tests that wouldn't properly clean up or reconcile everything before shutting stuff down (including the event recorder, which panics).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, vincepri The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/milestone v0.9.x |
/retest |
1 similar comment
/retest |
Currently, the controller will instantly shutdown and return when its
context gets cancelled, leaving active reconciliations be. This change
makes it wait for those before finishing shutdown.
Fixes #1424